Skip to content

Reapply "[FRONTEND] added support for tuples (#5220)" #3043

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 12 commits into from
Dec 21, 2024
Merged

Reapply "[FRONTEND] added support for tuples (#5220)" #3043

merged 12 commits into from
Dec 21, 2024

Conversation

@anmyachev
Copy link
Contributor

@anmyachev anmyachev commented Dec 18, 2024 •
edited
Loading

This reverts commit 492ea92.

Summary of changes:

  • support for tuples
  • constexprs are now also part of the signature. The format is: signature={..., 'o': "*fp32", '[Name of constexpr]': 'constexpr'}
  • For ASTSource there is parameter name change: constants -> constexprs
  • New nesting level has appeared for the main properties (defined in _add_common_properties): divisibility_16, equal_to_1 and equal_to_none.

Inductor CI:

anmyachev and others added 8 commits December 18, 2024 20:41
@anmyachev
Copy link
Contributor Author

anmyachev commented Dec 19, 2024

On Triton's side I think I fixed everything.

For Torch Inductor, even considering that I made several changes for it, this is still not enough and the full set of inductor tests does not pass. Considering the number of changes, I believe it is good idea to split it into another PR.

@anmyachev anmyachev marked this pull request as ready for review December 19, 2024 20:57
@anmyachev anmyachev requested review from alexbaden and whitneywhtsang and removed request for alexbaden December 19, 2024 20:57
@alexbaden
Copy link
Contributor

alexbaden commented Dec 19, 2024

For Torch Inductor, even considering that I made several changes for it, this is still not enough and the full set of inductor tests does not pass. Considering the number of changes, I believe it is good idea to split it into another PR.

I agree. I think we should remove the PyTorch patch from this PR and keep it in a separate branch since the inductor tests will fail either way.

whitneywhtsang
whitneywhtsang reviewed Dec 19, 2024
View reviewed changes
Copy link
Contributor

@whitneywhtsang whitneywhtsang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the discussion, are we removing all debug prints and PyTorch patches in this PR?

@anmyachev
Copy link
Contributor Author

anmyachev commented Dec 20, 2024

@alexbaden I can't completely remove the patch, because even the main tests will stop working. We have at least one test in the main set that uses PyTorch inductor:

intel-xpu-backend-for-triton/scripts/test-triton.sh

Line 341 in b9afb6c

ZE_AFFINITY_MASK=0 python pytorch/benchmarks/dynamo/huggingface.py --accuracy --float32 -dxpu -n10 --no-skip --dashboard --inference --freezing --total-partitions 1 --partition-id 0 --only AlbertForMaskedLM --backend=inductor --timeout=4800 --output=$(pwd -P)/inductor_log.csv

@whitneywhtsang I removed debug things from the Triton code, but I intend to leave them in the patch, because they will still be useful for fixing the inductor tests (at least they highlight places that are affected by the change in the Triton interface).

UPD: CI is not working now because there is no more space on the runners: /runner/_work/intel-xpu-backend-for-triton/intel-xpu-backend-for-triton/pytorch/third_party/ideep/mkl-dnn/src/gpu/intel/jit/gemm/gen_gemm_kernel_generator.cpp:27738:1: fatal error: error writing to /tmp/ccXBqNWT.s: No space left on device. cc @kwasd @pbchekin

@anmyachev
Copy link
Contributor Author

anmyachev commented Dec 20, 2024

@pbchekin @kwasd please ping me when it makes sense to try to restart CI

@pbchekin
Copy link
Contributor

pbchekin commented Dec 20, 2024

@pbchekin @kwasd please ping me when it makes sense to try to restart CI

Please restart.

@anmyachev
Copy link
Contributor Author

anmyachev commented Dec 20, 2024

@pbchekin @kwasd please ping me when it makes sense to try to restart CI

Please restart.

Unfortunately it doesn't work

@pbchekin
Copy link
Contributor

pbchekin commented Dec 20, 2024

Unfortunately it doesn't work

Ops, sorry for that. Let me fix the problem, merge #3053, then I restart the workflow.

@anmyachev
Copy link
Contributor Author

anmyachev commented Dec 21, 2024

Test with pip failed in the Inductor test suite, and this happened because pytorch is not rebuilt with the new patch in this workflow. So I'm going to merge this pull request, assuming that as soon as new PyTorch wheels become available for this flow, it should go through successfully.

@anmyachev anmyachev merged commit 2347dba into main Dec 21, 2024
5 of 6 checks passed
@anmyachev anmyachev deleted the amyachev/issue2979 branch December 21, 2024 10:46
@anmyachev anmyachev mentioned this pull request Dec 21, 2024
Closed
anmyachev added a commit that referenced this pull request Dec 21, 2024
@anmyachev
Hot fix for 'benchmark_driver.py' after #3043
6eb3963 
Signed-off-by: Anatoly Myachev <[email protected]>
whitneywhtsang pushed a commit that referenced this pull request Dec 21, 2024
@anmyachev
Hot fix for benchmark_driver.py after #3043 (#3060)
e69f985 
Signed-off-by: Anatoly Myachev <[email protected]>
@anmyachev anmyachev mentioned this pull request Dec 24, 2024
Closed
anmyachev added a commit that referenced this pull request Jan 4, 2025
@anmyachev
Fix Pytorch inductor tests workflow since it doesn't work due to Trit…
8f1f00e 
…on interface change in #3043 (#3080)

Signed-off-by: Anatoly Myachev <[email protected]>
@kballeda kballeda mentioned this pull request Jan 6, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@whitneywhtsang whitneywhtsang whitneywhtsang approved these changes

+1 more reviewer

@alexbaden alexbaden alexbaden approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@anmyachev @alexbaden @pbchekin @whitneywhtsang